fix(engine): record a token battlefield entry even when its events are suppressed (CR 403.3) - #6851
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSuppressed copy-token entries now park battlefield-entry state until copy and as-enters processing settles. Shared helpers record and emit each entry once. Coverage reports now include the head commit SHA. ChangesDeferred Token Battlefield Entry
Coverage Report Provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CopyTokenResolution
participant GameState
participant TokenEntryHelpers
participant PriorityPipeline
CopyTokenResolution->>GameState: Park suppressed token entry
CopyTokenResolution->>TokenEntryHelpers: Schedule object-only emission
TokenEntryHelpers->>GameState: Flush settled battlefield entry
TokenEntryHelpers->>PriorityPipeline: Emit entry events for scanning
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
There was a problem hiding this comment.
Blocking: record_committed_token_entry now writes the Suppress-route rows before BecomeCopy, but their content is refreshed only by emit_recorded_token_entry_events in the fully unpaused tail. That misses the very mid-entry pauses this change says it supports.
For example, an Embalm Vizier copying Painter's Servant reaches finish_copy_target_choice_entry after BecomeCopy; current_self_enter_replacement_choice then raises Painter's mandatory color NamedChoice and this function returns before the emit/refresh at the caller's tail. The NamedChoice resume path replays deferred entry events, but it does not invoke emit_recorded_token_entry_events. The two ledger rows therefore remain the pre-copy 0/0 Vizier snapshot instead of the realized Painter's Servant entry.
That is materially incorrect for the CR 400.7/403.3 lookup consumers this PR is updating, and it is the opposite of the unpaused Bear assertion added here. Please carry the record refresh through each post-copy pause/resume (or postpone record construction until the copied characteristics and mandatory as-enters choices are complete), then add an integration regression that drives this copied mandatory-choice pause and asserts both ledgers describe the realized copy exactly once.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/token.rs`:
- Around line 1835-1891: Centralize battlefield-entry recording through
record_zone_change for the production paths in counters.rs, token_copy.rs, and
gift_delivery.rs, ensuring each entry creates both synchronized ledger rows
before emitting ZoneChanged events. Update emit_recorded_token_entry_events to
detect and repair or reject mismatched ledgers, specifically preventing the
(None, Some(_)) case from appending a duplicate battlefield-entry record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d51e64b-761f-4273-9f97-c6bed7a3d8b8
📒 Files selected for processing (4)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine_replacement.rscrates/engine/tests/integration/token_zone_change_index.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
🤖 AI text below 🤖 Confirmed, and it is worse than you stated. I reproduced your exact path as an integration Two things I did not know before measuring: 1. The emit is lost on this route too, not just the refresh. 2. Baseline separation. Same probe grafted onto merged
So the missing emit is pre-existing and unchanged by this PR; what this PR changes on this route is Also confirmed: CodeRabbit's related ledger-mismatch Minor. Seven production callers write Taking your second option (postpone construction until the copied characteristics and the Marking this PR draft while that is in flight so it is not sitting on your queue. |
|
Thanks for reproducing this and for separating the baseline behavior from the regression introduced here. I confirmed the follow-up against the unchanged head: it does not resolve the requested change, and the current The Paint-er mandatory-choice path establishes both failures on this head: the newly added rows retain the pre-copy identity, and the deferred entry emit is dropped. Your confirmation of CodeRabbit's Keeping this draft while you rework it is appropriate. Please push the redesigned implementation and the production integration regression before requesting another review. |
…eld entries CR 400.7 + CR 403.3. A `TokenEntryEventEmission::Suppress` token's battlefield entry was recorded by the finalize tail BEFORE `BecomeCopy` resolved and its content refreshed only in `handle_copy_target_choice`'s fully-unpaused tail, so every mid-entry pause kept a pre-copy row — and four of the five pause returns in `finish_copy_target_choice_entry` never reached the deferred emit at all. Measured on the maintainer's named path (Embalm Vizier of Many Faces copying Painter's Servant, whose mandatory as-enters `NamedChoice` returns from `finish_copy_target_choice_entry`): both ledgers held a pre-copy `Vizier of Many Faces` 0/0 row and no entry event was emitted. Replace the split record-here / refresh-there route with one postponed lifecycle: - The `Suppress` finalize tail RECORDS NOTHING. It parks the entry on `GameState::pending_token_battlefield_entry` (serde-persisted, so it survives arbitrary client round trips — CR 614.12a puts the as-enters choice before the permanent enters). - `token::flush_pending_token_battlefield_entry` is the sole consumer. It takes the parked value with `Option::take_if` and, over that one owned value, calls `restrictions::record_zone_change` — the single writer of BOTH CR 400.7 ledgers — and pushes the CR 603.6a entry pair. "Recorded but never emitted", "emitted but never recorded", and the duplicate battlefield-entry row are unrepresentable rather than guarded. - `emit_recorded_token_entry_events` is deleted. Its `(None, Some(_))` arm — which appended a duplicate `battlefield_entries_this_turn` row, reported by CodeRabbit — no longer has a representation: nothing reads a previously-written row back. Three convergence points feed that one authority, and none subsumes another (each is revert-probed): - `finish_copy_target_choice_entry`, for the unpaused route, whose action ends on a stale `CopyTargetChoice` and so would otherwise realize one client round trip late; - the `EmitCommittedCopyTokenEntry` post-action, inside the CR 616.1 counter drain, ahead of that action's trigger scan; - `token::realize_settled_token_battlefield_entry`, one gate (settled `WaitingFor::Priority` + the token still on the battlefield) called from two sites in `engine.rs`: inside `apply_action` immediately before `run_post_action_pipeline`, so the realized pair is trigger-scanned in its own action (CR 603.6a — Soul Warden observes the copy token), and again at the action boundary as the backstop for handlers that return an `ActionResult` straight out of the reducer match. The settled gate is pause-shape-agnostic by construction: it names the settled state, the complement of "any pause", so no `WaitingFor` variant appears in it. That is what covers the as-enters class whose continuation raises a SECOND pause (11 Tribute creatures and the `RevealHand` chains), which a resume-arm hook would have missed. Known partial, measured and pinned: `handle_tribute_choice` builds its `ActionResult` directly out of the reducer match, so the two-pause class realizes at the boundary — after the trigger scan — and its ETB observers do not fire (Soul Warden delta 0). Both ledgers and the emit are correct there; this is strictly better than the previous behaviour, which emitted nothing at all for that class, and it is a property of the reducer's direct-return handlers rather than of this lifecycle. Regressions drive the real cast pipeline and pin all four routes, each with a reach-guard on the exact prompt sequence: the Painter's Servant mandatory-choice pause (post-copy identity on both ledgers exactly once, a non-empty emitted pair carrying the recorder-assigned index, and the ETB observer at +1), the Fanatic of Xenagos two-pause class, the CR 616.1 counter-ordering pause, and the unpaused route; plus a positive control on the untouched `Emit` path. Reported by matthewevans (phase-rs#6851 review) and CodeRabbit; both verified before fixing. Assisted-by: ClaudeCode:claude-opus-5
1bd2705 to
a53bde0
Compare
|
🤖 AI text below 🤖 Redesigned and pushed. I took your second option — postpone construction until the copied The shapeThe
Three convergence points feed that one authority; each is independently revert-probed, and none
(c)'s gate is pause-shape-agnostic by construction: it names the settled state, the complement of Measured, on your named pathEmbalm Vizier of Many Faces → copy Painter's Servant → mandatory colour
One correction to the review, and one accepted partialCorrection. Your review says the Accepted partial, pinned rather than left silent. Two pre-existing defects found while measuring — filed, not folded in
Also noted, not acted on: ProcessFull Disclosure: round 2's plan-review finding was a prose-only correction to the plan document, applied |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/tests/integration/token_zone_change_index.rs (1)
808-848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
token == Nonecase explicit so per-step counters cannot read as vacuous zeros.
matches_tokenreturnsfalsefor every event and every ledger row whentokenisNone. On the declined routedrive.tokenis never set, so all four counters inCopyEntryStepare zero by construction rather than by measurement.declined_copy_replacement_records_the_token_entry_without_parking_itreads the ledgers fromrunner.state()directly and only usesstepsforparked, so it is not blind today. A later per-step assertion on that route would pass for the wrong reason.Consider making the unmeasurable case unrepresentable, for example by storing
Option<TokenCounts>inCopyEntryStepwhen the token id is unknown, or by documenting on the struct that counters are meaningless whiletokenisNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/token_zone_change_index.rs` around lines 808 - 848, Make the unknown-token case explicit in token_entry_step and CopyEntryStep so per-step counters cannot appear as measured zeroes when token is None. Prefer representing the counter group as Option<TokenCounts> (or an equivalent explicit unmeasurable state), returning None when no token id is available and preserving normal counts for Some(token); update consumers and assertions to unwrap or handle that state while keeping parked independently measurable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/turns.rs`:
- Around line 1161-1166: Update the comment immediately above
pending_token_battlefield_entry’s turn-boundary reset to cite CR 514.2 as the
authority for ending “this turn” state, while retaining CR 400.7 and CR 603.6a
for entry realization and ETB-trigger behavior.
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 1094-1095: Update the dedup-key descriptions near the referenced
comments to remove the “CR 603.2c” citation and describe the turn-zone-change
index and batched-replay deduplication as engine implementation details. Apply
the same wording correction to the corresponding description near the later
occurrence, without changing the behavior or surrounding test logic.
---
Nitpick comments:
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 808-848: Make the unknown-token case explicit in token_entry_step
and CopyEntryStep so per-step counters cannot appear as measured zeroes when
token is None. Prefer representing the counter group as Option<TokenCounts> (or
an equivalent explicit unmeasurable state), returning None when no token id is
available and preserving normal counts for Some(token); update consumers and
assertions to unwrap or handle that state while keeping parked independently
measurable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd662eb-8fa2-4ac5-b1dd-476d8aa3db9a
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/scenario_db.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/token_zone_change_index.rs
CodeRabbit on phase-rs#6851. The per-turn clear of `pending_token_battlefield_entry` cited only CR 400.7, which names the ledgers it defends but is not the authority for the reset itself. CR 514.2 is: the cleanup step is where all "until end of turn" and "this turn" effects end. Cite both. Also tightens the one place that read as though CR 603.2c names `turn_zone_change_index`. The index is the engine's own key; the batched zone-change replay guard dedups on it to hold CR 603.2c's once-per-occurrence bound. The citation stays — it is the rule the guard implements, and it matches this file's existing module header — but the sentence now separates rule from mechanism. Comment-only; no behaviour change. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @coderabbitai — both addressed in 1. 2. The citation itself stays, for two measured reasons. First, this file already establishes that No change at line 1382, and here I think the finding does not apply. That doc comment opens Thanks also for the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline bee984f80
git show --format=fuller --unified=12 bee984f80 -- \
crates/engine/src/game/turns.rs \
crates/engine/tests/integration/token_zone_change_index.rs
printf '\n--- turns.rs context ---\n'
nl -ba crates/engine/src/game/turns.rs | sed -n '1150,1185p'
printf '\n--- token_zone_change_index.rs contexts ---\n'
nl -ba crates/engine/tests/integration/token_zone_change_index.rs | sed -n '1070,1120p'
nl -ba crates/engine/tests/integration/token_zone_change_index.rs | sed -n '1360,1400p'
printf '\n--- annotation guidance ---\n'
fd -a -t f '^CLAUDE\.md$' . -x sh -c 'echo "--- $1 ---"; rg -n -C3 "annotation|rule|citation|CR 603\\.2c|turn_zone_change_index" "$1" || true' sh {}Length of output: 6987 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current Line 1382 context ---'
sed -n '1360,1405p' crates/engine/tests/integration/token_zone_change_index.rs
printf '%s\n' '--- module header and prior CR 603.2c annotations ---'
sed -n '1,55p' crates/engine/tests/integration/token_zone_change_index.rs
grep -n -C 2 -E 'CR 603\.2c|turn_zone_change_index' \
crates/engine/tests/integration/token_zone_change_index.rs || true
printf '%s\n' '--- CLAUDE.md guidance ---'
find . -name CLAUDE.md -type f -print -exec sh -c '
echo "--- $1 ---"
grep -n -i -C 3 -E "annotation|rule|citation|MagicCompRules|definition_ref" "$1" || true
' sh {} \;Length of output: 46071
The The comment at Line 1095 now separates I withdraw the Line 1382 finding. The existing comment already makes the same distinction. It states the CR 603.2c requirement first, then identifies 🐇 You are interacting with an AI system. |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new parked-entry lifecycle records the row, but still drops ETB trigger emission on a reachable two-pause continuation.
🔴 Blocker
crates/engine/src/game/effects/token.rs:1904-1908 explicitly documents that the handle_tribute_choice direct-return path realizes the parked entry only at the action boundary, after its trigger scan. crates/engine/src/game/engine.rs:376-385 confirms that placement. The production-path test in crates/engine/tests/integration/token_zone_change_index.rs:1150-1161 reaches CopyTargetChoice → NamedChoice → TributeChoice, then deliberately asserts Soul Warden’s life delta is 0 at :1213-1225. That result is rules-incorrect: CR 603.6a says ETB abilities are checked whenever an event puts a permanent onto the battlefield (verified local corpus docs/MagicCompRules.txt:2599; CR 603.2c at :2567). Make the direct-return continuation converge through the normal post-action trigger pipeline, or flush before that handler’s trigger scan; the regression must assert Soul Warden gains 1.
🔴 Required evidence
The required <!-- coverage-parse-diff --> comment for current engine source is not bound to the head: issue comment 5153742352, updated for this run, says only “No card-parse changes detected” with no SHA. Regenerate/update the artifact so it identifies bee984f809e084d2bd0c71c4bbbb3d67ac8d13b4.
✅ Clean
The prior stale/lost Suppress-route ledger finding is addressed by the park/flush design; the old CodeRabbit lifecycle finding is stale. Current CI is green, but it does not remedy the reachable trigger defect.
Recommendation: request changes on this head, then resubmit with the trigger-pipeline fix, a discriminating +1 ETB assertion, and a head-bound parse-diff artifact.
…eld entries CR 400.7 + CR 403.3. A `TokenEntryEventEmission::Suppress` token's battlefield entry was recorded by the finalize tail BEFORE `BecomeCopy` resolved and its content refreshed only in `handle_copy_target_choice`'s fully-unpaused tail, so every mid-entry pause kept a pre-copy row — and four of the five pause returns in `finish_copy_target_choice_entry` never reached the deferred emit at all. Measured on the maintainer's named path (Embalm Vizier of Many Faces copying Painter's Servant, whose mandatory as-enters `NamedChoice` returns from `finish_copy_target_choice_entry`): both ledgers held a pre-copy `Vizier of Many Faces` 0/0 row and no entry event was emitted. Replace the split record-here / refresh-there route with one postponed lifecycle: - The `Suppress` finalize tail RECORDS NOTHING. It parks the entry on `GameState::pending_token_battlefield_entry` (serde-persisted, so it survives arbitrary client round trips — CR 614.12a puts the as-enters choice before the permanent enters). - `token::flush_pending_token_battlefield_entry` is the sole consumer. It takes the parked value with `Option::take_if` and, over that one owned value, calls `restrictions::record_zone_change` — the single writer of BOTH CR 400.7 ledgers — and pushes the CR 603.6a entry pair. "Recorded but never emitted", "emitted but never recorded", and the duplicate battlefield-entry row are unrepresentable rather than guarded. - `emit_recorded_token_entry_events` is deleted. Its `(None, Some(_))` arm — which appended a duplicate `battlefield_entries_this_turn` row, reported by CodeRabbit — no longer has a representation: nothing reads a previously-written row back. Three convergence points feed that one authority, and none subsumes another (each is revert-probed): - `finish_copy_target_choice_entry`, for the unpaused route, whose action ends on a stale `CopyTargetChoice` and so would otherwise realize one client round trip late; - the `EmitCommittedCopyTokenEntry` post-action, inside the CR 616.1 counter drain, ahead of that action's trigger scan; - `token::realize_settled_token_battlefield_entry`, one gate (settled `WaitingFor::Priority` + the token still on the battlefield) called from two sites in `engine.rs`: inside `apply_action` immediately before `run_post_action_pipeline`, so the realized pair is trigger-scanned in its own action (CR 603.6a — Soul Warden observes the copy token), and again at the action boundary as the backstop for handlers that return an `ActionResult` straight out of the reducer match. The settled gate is pause-shape-agnostic by construction: it names the settled state, the complement of "any pause", so no `WaitingFor` variant appears in it. That is what covers the as-enters class whose continuation raises a SECOND pause (11 Tribute creatures and the `RevealHand` chains), which a resume-arm hook would have missed. Known partial, measured and pinned: `handle_tribute_choice` builds its `ActionResult` directly out of the reducer match, so the two-pause class realizes at the boundary — after the trigger scan — and its ETB observers do not fire (Soul Warden delta 0). Both ledgers and the emit are correct there; this is strictly better than the previous behaviour, which emitted nothing at all for that class, and it is a property of the reducer's direct-return handlers rather than of this lifecycle. Regressions drive the real cast pipeline and pin all four routes, each with a reach-guard on the exact prompt sequence: the Painter's Servant mandatory-choice pause (post-copy identity on both ledgers exactly once, a non-empty emitted pair carrying the recorder-assigned index, and the ETB observer at +1), the Fanatic of Xenagos two-pause class, the CR 616.1 counter-ordering pause, and the unpaused route; plus a positive control on the untouched `Emit` path. Reported by matthewevans (phase-rs#6851 review) and CodeRabbit; both verified before fixing. Assisted-by: ClaudeCode:claude-opus-5
CodeRabbit on phase-rs#6851. The per-turn clear of `pending_token_battlefield_entry` cited only CR 400.7, which names the ledgers it defends but is not the authority for the reset itself. CR 514.2 is: the cleanup step is where all "until end of turn" and "this turn" effects end. Cite both. Also tightens the one place that read as though CR 603.2c names `turn_zone_change_index`. The index is the engine's own key; the batched zone-change replay guard dedups on it to hold CR 603.2c's once-per-occurrence bound. The citation stays — it is the rule the guard implements, and it matches this file's existing module header — but the sentence now separates rule from mechanism. Comment-only; no behaviour change. Assisted-by: ClaudeCode:claude-opus-5
The reducer's direct-return arms build their `ActionResult` inside the match and never reach `apply_action`'s tail pipeline, so a CR 403.3 token battlefield entry realized on one of those routes entered with no ETB observer ever seeing it — CR 603.6a requires the check to run whenever an event puts a permanent onto the battlefield. `realize_settled_token_battlefield_entry` now reports whether it realized, and `apply_action_boundary_core` runs the same `run_post_action_pipeline_from` the tail path delegates to, over the slice the realization appended. The convergence is arm-agnostic: the match has 16 non-error direct returns and this is inert on the 15 that never park. Measured on the reviewer's named path (Embalm Vizier of Many Faces -> copy Painter's Servant -> mandatory colour NamedChoice -> Tribute, Soul Warden on the battlefield): Soul Warden's life delta goes 0 -> 1. Reverting the block reproduces the 0, failing at the reach-guard first; with the guard relaxed the life assertion alone flips. The regression pins "OrderTriggers(2)" — Tribute is declined, so Fanatic's CR 702.104b intervening-if is true and its ETB fires alongside Soul Warden's, both P0's, so CR 603.3b requires an ordering prompt. Both `waiting_for` writes are required: `finish_action_boundary` -> `sync_waiting_for` copies the result into the state, so a state-only write is undone, and the life-safety preview never calls `finish_action_boundary`, so a result-only write is insufficient. Assisted-by: ClaudeCode:claude-opus-5
`render_markdown` returned early for the no-change case with the marker and "No card-parse changes detected" and nothing else — no base, no head — while the changed-cards branch printed only the baseline. No branch had ever emitted a head SHA, so the sticky comment could not be tied to the commit it described and regenerating it could not help. Both branches now carry the head, sourced from the `HEAD_SHA` the CI step already exports, with a `--head-sha` flag mirroring the existing `--base-sha`. Deriving it from git would be wrong rather than merely inelegant: that job checks out the synthetic PR merge commit, so git reports the merge SHA, never the PR head. Two consumer constraints shape the wording, each pinned by an assertion with a revert-probe: `scripts/pr_review.py` requires the marker to remain the first line, and it classifies sticky state by the substrings "Baseline pending" and "signature(s)", so the head line must contain neither or a no-change sticky misclassifies as real parse changes. Assisted-by: ClaudeCode:claude-opus-5
bee984f to
35f71f0
Compare
|
🤖 AI text below 🤖 Thanks — both items are addressed at 🔴 Blocker — direct-return continuation now converges through the post-action pipelineYou offered two remedies. I measured the second one first, and it has nothing to attach to:
Measured on the path you named (
The regression at One residual I want on the record rather than discovered later: the pipeline runs from 🔴 Required evidence — the sticky could not identify the head, and regenerating cannot fix itI could not satisfy this by regenerating, and the reason is a defect rather than a stale run. Comment
That generator is ordinary crate source, not a workflow, so I fixed it there: both branches now carry the head, sourced from the Two constraints shaped the wording, both pinned by assertions with revert-probes: One consequence worth stating so it doesn't cost you a round: the sticky names the head it was generated from, so landing these commits necessarily moves it. The regenerated comment will identify This is a class-wide provenance fix rather than a #6851 one, so it is disclosed under Scope Expansion. Two residuals remain that I deliberately did not touch, because they live in
Credit to CodeRabbit for the CR 514.2 citation correction on the per-turn reset in the earlier round. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/bin/coverage_parse_diff.rs`:
- Line 347: Update the argument parsing branch for --head-sha so a present
option with no following value returns a usage error instead of reusing the
existing head_sha fallback; preserve accepting and assigning a supplied value.
Add a regression test covering trailing --head-sha and assert that parsing fails
with the expected usage error.
- Line 445: Update the DiffReport data model to include the head_sha provenance
field, populate it when constructing the report in the coverage diff parser, and
extend the associated tests and expected parse-diff.json output to verify the
field is serialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bef5dcf4-b52c-450e-940a-9a54d0e2e72b
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (10)
crates/engine/src/bin/coverage_parse_diff.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/scenario_db.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/token_zone_change_index.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/engine/src/game/scenario_db.rs
- crates/engine/src/game/turns.rs
- crates/engine/src/game/elimination.rs
- crates/engine/src/game/effects/counters.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/engine_replacement.rs
- crates/engine/src/game/engine.rs
- crates/engine/tests/integration/token_zone_change_index.rs
- crates/engine/src/game/effects/token.rs
Two review findings from CodeRabbit on the head-SHA change. A present-but-valueless `--base-sha` or `--head-sha` silently fell back to the previous value, so a report could name a commit it does not describe. Both now return a usage error. The pre-existing `--base-sha` line is fixed alongside the one this branch added: leaving it lenient would make two flags in the same provenance category behave differently. `--markdown`, `--json` and `--max-clusters` stay lenient by design and a test pins that asymmetry as deliberate — a missing value there omits output the caller can see, so there is nothing to misattribute. `parse-diff.json` carried no provenance at all, while both the Markdown report and the sticky comment direct a reader with truncated output to open it. `DiffReport` now carries `head_sha` and `base_sha`, ordered as the Markdown presents them. Nothing in the repo deserializes that artifact — the four references to it are two workflow write/upload paths and two prose mentions — so the added keys cannot break a consumer. Neither defect is reachable through CI, which never passes `--head-sha` and always pairs its flags with values; this hardens the local and manual surface. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @coderabbitai — both taken, in Missing One correction on the impact, offered as measurement rather than disagreement about the fix: it is not reachable from CI. Provenance in Verification: three new tests (bin suite 9 → 12), each with a revert-probe that was run and restored byte-identically. The JSON probe swaps head and base rather than deleting a key, so it fails on a symmetric mistake and not merely a missing field. Full suite One related weakness I found while doing this and deliberately did not fix: all five flags still greedily consume the next token, so |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline bc9f94455
printf '\n--- changed coverage tool diff ---\n'
git show --format=fuller --unified=10 bc9f94455 -- crates/engine/src/bin/coverage_parse_diff.rs
printf '\n--- source outline ---\n'
ast-grep outline crates/engine/src/bin/coverage_parse_diff.rs --items all
printf '\n--- option parsing and provenance contexts ---\n'
rg -n -C 5 -- '--base-sha|--head-sha|struct DiffReport|head_sha|base_sha|missing.*value|usage' \
crates/engine/src/bin/coverage_parse_diff.rs
printf '\n--- related file references ---\n'
rg -n -C 3 'parse-diff\.json|DiffReport|head_sha|base_sha' \
crates/engine/src/bin/coverage_parse_diff.rs scripts .github 2>/dev/null || trueLength of output: 81965
The Major finding is addressed. The Minor finding is addressed. Your CI-scope correction is accurate. The change hardens manual and local invocation. The remaining greedy-next-token case is separate from these two findings. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/bin/coverage_parse_diff.rs`:
- Around line 365-366: Update the argument parsing branches for --base-sha and
--head-sha to reject missing, empty, and values beginning with "--" instead of
accepting them as SHA strings. Preserve valid SHA parsing, and add tests
covering empty and option-token values for both flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8643ffe7-4c30-421c-b441-2bf6b0c57ff3
📒 Files selected for processing (1)
crates/engine/src/bin/coverage_parse_diff.rs
Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
|
Maintainer hold for current head |
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current head passes required CI, the parse-diff sticky is bound to this head with no card-parse changes, and the previously requested lifecycle/provenance corrections are verified.
|
🤖 AI text below 🤖 Thanks — and thanks for taking the fixup directly. Your Do not hold the merge for anything below. Two residuals survive your validation. Neither is reachable from Measured on your binary at
If you want them, I have both as a tested patch (2 tests, revert-probes run and restored byte-identically) and will open it as a separate PR against main after this lands — normalizing blank-or-whitespace to |
…nto phase-rs#6851 CI built the MERGE ref (branch + main 96e41b3) while the branch was still based on e12447f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by phase-rs#6851's own insertions above this producer: `git diff -U0 e12447f 96e41b3` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and phase-rs#6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5
…nto phase-rs#6851 CI built the MERGE ref (branch + main 96e41b3) while the branch was still based on e12447f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by phase-rs#6851's own insertions above this producer: `git diff -U0 e12447f 96e41b3` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and phase-rs#6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5
…nto phase-rs#6851 CI built the MERGE ref (branch + main 96e41b3) while the branch was still based on e12447f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by phase-rs#6851's own insertions above this producer: `git diff -U0 e12447f 96e41b3` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and phase-rs#6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5
…firewall, and measured 4p rows (combo-fb phases 5a-5d, chain 3) (phase-rs#6886) * feat(engine): pin per-iteration decision slots for bounded-loop shortcut A bounded cycle can only be fast-forwarded when every player choice it repeats is forced. Publish the pinned slots for the one class where that provably holds — a "target opponent" triggered ability — and let the loop-window scope discharge exactly those, and only those. The published legal set is MOVED from the announcement authority (`build_target_slots`, CR 601.2c: the ability's controller announces), never re-derived from the head effect's target filter. Re-deriving it published [Player(1), Player(2)] where the builder said [Player(0), Player(1), Player(2)] on a chained "target player" sub-ability — measured, not hypothetical. The head filter is now a shape gate only, enforced by a `bool` return rather than by comment, so re-deriving it is unrepresentable. The relief returns a residual verdict instead of a blanket `continue`, so it discharges only what the pin specifies and re-arms the life guard; and the re-check delegates to the same authority that minted the value, so the two cannot drift. CR 601.2c / CR 603.3d / CR 115.2 (each grepped in docs/MagicCompRules.txt) Tests: 22036 passed / 0 failed / 15 ignored (`cargo test -p engine`; the 15 is 8 #[ignore] + 7 doc-test fences. `cargo nextest run -p engine` reports 22036 passed / 8 skipped — it does not run doc-tests, so cite the runner). clippy --workspace --all-targets -D warnings: clean. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): bounded CR 732.2a cycle fast-forward for a multiplayer drain A draining loop never returns the board to a prior state, so board-recurrence alone cannot certify it. Add a second, board-blind basis: a periodic resource signature derived from the retained ring, which certifies a period only after seeing it twice, and publish a bounded shortcut whose iteration cap is the CR 704.5a elimination minimum computed from the live board. The board-blind basis needs a discriminant the board-based one gets for free. Without one it accepts the game's own turn structure — two players each drawing once per turn is a genuine repeating resource signature, and it minted a shortcut offer on a board with no loop at all. CR 732.2a permits a shortcut to cross turns, so the fix is scoped to the board-blind basis only: its certifying window must be turn-position invariant, expressed through the same window_scope_from_cover_frames authority the growing-class firewall already uses. Zero new predicates, types, or fields. Basis attribution is by discriminating probe, never by frames_per_period: basis A publishes 1 unconditionally and the board-blind basis at k==1 publishes the identical value, so equality discriminates nothing. That inference had already mislabeled a real four-player drain dump as board-recurrent; it is in fact the board-blind basis, and it is this feature's real-dump control. On a growing cascade the two bases are separated by whether the board carries a fire-time condition reading a projected resource axis -- not by which resources move. Recorded at the dispatch site, since both shipped row docs had asserted a resource-purity rule that measurement refuted on their own fixtures. CR 732.2a / CR 704.5a / CR 703.1a / CR 601.2c (each grepped in docs/MagicCompRules.txt) Tests: 22046 passed / 0 failed / 15 ignored (cargo test -p engine); cargo nextest run -p engine reports 22046 passed / 8 skipped -- it does not run doc-tests, so cite the runner. clippy --workspace --all-targets -D warnings clean. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): a mandatory draw is not a resolution-time choice The bounded-shortcut offer requires every stack entry's resolution-time choices to be specified. That verdict came from an allow-list of exactly two Effect variants -- GainLife and LoseLife -- out of roughly two hundred, with everything else falling to a fail-closed MayPrompt. The exclusions were never assessed and found choice-bearing; they were simply never enumerated, so a draining loop whose stack held a draw trigger could never be fast-forwarded. Keyed on the MECHANISM, never the variant name: a mandatory draw whose only prompt route is its replacement environment. The same Effect::Draw is genuinely choice-bearing when its ability is optional (CR 603.5), and stays refused; a count of "up to" is a CR 608.2d resolution-time choice and is excluded fail-closed, recursively, so a future QuantityExpr variant cannot smuggle one past a wildcard. The verdict is parameterized rather than given a sibling: FreeUnlessReplacements carries the set of replacement classes its obligation covers, and chained abilities union them. A FreeUnlessDrawReplacements sibling would have forced all three consumers to re-derive that union. The parameterization is grounded, not assumed: all three prompt seams live in the event-agnostic pipeline_loop and none branches on event class. Also corrects a premise this code documented and a real fixture falsifies. The every-entry scan was justified by "in an exact-recurrence window every entry re-announces each cycle"; the dellian dump is a growing-cascade window over a frozen bottom prefix -- 107 entries with identical ids and indices surviving 220 beats while the stack grew above them. dellian is named there as the refutation only. No allow-list widening of any size unblocks it, and nothing here claims otherwise. CR 121.1 / CR 616.1 / CR 608.2d / CR 603.5 / CR 702.52a / CR 704.5b (each grepped in docs/MagicCompRules.txt; 702.52a is dredge -- 702.51a, written from memory first, is convoke) Acceptance: bloodloop at 3 and 4 players goes 0 offers -> 1, with UnspecifiedChoiceWindow 34 -> 0 attributing the flip to that gate; the 2p offer moves beat 31 -> 29. All three beats pinned. Five revert probes, each flipping its own assertion. Tests: 22048 passed / 0 failed / 15 ignored (cargo test -p engine); cargo nextest run -p engine reports 22048 passed / 8 skipped -- it does not run doc-tests, so cite the runner. clippy --workspace --all-targets -D warnings clean. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): delimit a bounded CR 732.2a cycle by its published per-period frame span A basis-B certificate — what `ring_delta_signature` mints, and the class the bounded offer widened to — certifies a periodic DELTA, not a recurring board. `materialize_fixed_shortcut`'s 'cycles loop only advanced on board recurrence, so neither recurrence predicate could ever fire and the declared `n` was structurally inert: the drive could only end at the beat cap (committing zero) or by crossing lethal. Measured on the production accept path at c6d834040, `Fixed(1)` and `Fixed(3)` were byte-identical — bloodloop 3p/4p wiped the whole table at either count, dina 4p committed nothing at either. `PeriodicDelta::frames_per_period` was written, cloned onto the proposal, and read by nothing. It is the missing delimiter, and its own doc already specified the check nobody implemented ('so a bounded drive can check that each committed cycle actually conformed'). Both now exist: * `drive_one_shortcut_cycle` takes the published span and completes a cycle once that many retained ring frames have been recorded. Frames are counted the same way they are minted — the engine's single `record_loop_detect_sample` call site is inside `pass_priority_once_with_pipeline`, the function the drive steps — and detected by Arc identity of the ring's back, since the ring evicts at its cap. * `materialize_fixed_shortcut` drops any committed cycle whose measured resource delta differs from the published one. CR 704.5a: `elimination_bounds` divided the headroom by that delta, so a divergent cycle invalidates the agreed bound. `per_cycle: None` (every pre-bounded offer) keeps board recurrence as the sole delimiter and skips the conformance check, so those drives are unchanged. Basis A's `frames_per_period` was a hardcoded `1` and is measured WRONG: the subset-lethal DRAIN_CLERIC/BLOOD_SIPPER fixture's repetition spans TWO frames. It is now derived from the certifying prior's ring index. Under the hardcode that fixture's accepted drive committed nothing at all (the new conformance check catching the half-period). The shipped `frames_per_period == 1` assertion was a self-ratifying oracle — it compared a literal against a constant no game state could influence — and is corrected to the measured 2 with that mechanism recorded. AFTER, same production path: bloodloop3 n=1 [20,17,17]->[20,16,16], n=3 ->[20,14,14], n=16 (the bound) ->[20,1,1]; dina 4p n=1 ->[50,34,30,35], n=30 (the bound) ->[79,5,1,6]. Zero eliminations at every n within bound. New rows: the n-scaling acceptance property on all three fixtures; the matched stop-short pair at the CR 704.5a boundary; the conformance drop; a named negative row for each of the three bounded-offer conjuncts that refused zero times; and the AI's bounded-declare candidate generated, applied and driven. Doc corrections, each re-measured rather than reasoned: the dina row's named step-(7) revert-probe genuinely does not flip (106/0 green under `1..=MAX`); the dellian frozen bottom prefix is 151 entries over 220 beats, not 140 or 107; the AI policy's 'every n within max_iterations eliminates nobody' premise was false when it shipped and is now true and re-measured. Assisted-by: ClaudeCode:claude-opus-4.8 * test(engine): cover the partial-crossing arm and the basis-A span at drive level Fix round 2 on the `review-impl` findings. No production behaviour changes: every `engine.rs` edit is a comment or lives inside `#[cfg(test)] mod bounded_offer_conjunct_tests`. MED-1. The required cross-lethal stop-short row was demonstrated only on bloodloop3, whose two opponents sit at equal life (17/17) and therefore cross 0 on the SAME cycle — a symmetric fixture cannot witness an asymmetric outcome. The arm real multiplayer boards take had no fixture, and the mirror's doc claim ("the eliminated set is exactly the seats the published period drains") is false on it. The two arms are asymmetric: total wipe every remaining opponent crosses together => WaitingFor::GameOver => CycleOutcome::CrossLethal: the crossing cycle COMMITS, game ends, eliminated == victims partial one seat crosses while >= 2 survive => no GameOver => CycleOutcome::Abort: the crossing cycle is ROLLED BACK WHOLE, eliminated == [] (by rollback, not because nobody crossed) Both are out of contract for a legitimately-derived bound — `elimination_bounds` reserves `life - 1` of CR 704.5a headroom — so each is reachable only under a doctored one. `bounded_fixed_drive_rolls_back_a_partial_crossing_cycle` asserts the measured partial behaviour on the dina 4p dump: honest bound 30, first crossings 31/35/36, so doctoring the bound to 31/34/40 commits exactly 30 periods, eliminates nobody and hands back `Priority{P0}`. The Abort is not "fixed": rolling the out-of-contract cycle back is conservative and correct, since the remaining repetitions were bounded by a delta the board stops moving once a drain target leaves. The mirror's claim is now scoped per arm and its arm-a doctoring is recorded — and asserted — as the no-op it is on that fixture. MED-2. Reverting `frames_per_period: span as u32` to the old hardcoded `1` flipped exactly one row, and that row asserts the PUBLISHED number, never accepting a `Fixed(n)` — so the basis-A half of the delimiter shipped on a single published-value assertion. `basis_a_bounded_fixed_count_commits_exactly_n_periods` declares and drives `Fixed(1)`, `Fixed(2)` and `Fixed(3)` through `apply()` on the subset-lethal (basis-A, k == 2) fixture and measures `n * delta` committed. Under the hardcode it commits `{0, 0, 0}` at every n — the conformance check dropping the half-periods a too-small k produces. The width tripwire is placed LAST on purpose: with it first, the hardcode probe failed there and the drive assertions were never reached, which would have made the row a second copy of the one it exists to back. Measured: the hardcode revert now flips 2 rows of 85, and the flip it reports is "the drive committed nothing". The `span >= 1` fail-closed guard gains `a_zero_span_certifying_pair_never_publishes_a_zero_width_period`, on a new `drain_ring` fixture. `mill_ring` could not serve: library size is BOARD, so its frames are board-unequal, basis A refuses them outright, and every mill-ring row in that module is really exercising basis B. Life is projected, so a drain ring's frames are board-equal and reach the basis-A walk. Reach-guards assert the span-0 pair satisfies both halves of the disjunct, so the guard is provably the refuser; deleting it publishes 0. Doc corrections, each re-measured. Six sites claimed "basis A publishes 1 unconditionally" — false since the previous commit made it a measured span, and one of them was carrying a basis attribution that inference no longer supports. Counts now name the runner AND the filter (`cargo test -p engine --test integration -- loop_shortcut::`, module filter, 4090 filtered out); the one bare count with neither recorded is deleted rather than re-dressed. Revert-probes run: delete the frame delimiter => 5 of 85 fail (the basis-A row does NOT, because its board recurs and delimits itself); delete the conformance check => 1 of 85 fails and the partial-crossing row stays green, which is what establishes its stop is the Abort rather than a conformance drop. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): the aborted cycle rolls back alone, not the whole drive Doc-only. Corrects the arm-asymmetry wording introduced in the previous commit, which read as if a partial crossing discarded the entire drive. It does not: `materialize_fixed_shortcut`'s `break 'cycles` falls through to `*state = committed`, the last WHOLE cycle — so the out-of-contract cycle is refused atomically while every prior conforming cycle stays committed. The measured shape already said so (dina 4p, honest bound 30: a doctored `n` at or past the first crossing commits 30 periods and refuses the 31st), and the row's assertions were already bound to `first_crossing - 1` rather than to zero. total wipe every remaining opponent crosses 0 on the same cycle => WaitingFor::GameOver => CycleOutcome::CrossLethal: the crossing cycle COMMITS, the game ends partial crossing one seat crosses 0 while >= 2 players survive => no GameOver => CycleOutcome::Abort: the crossing cycle rolls back WHOLE; prior conforming cycles STAY COMMITTED; priority handback The property this buys is named where it is implemented: no half-applied period, ever — refuse the out-of-contract cycle atomically, keep the conforming prefix. The per-arm eliminated-set split is unchanged and stays two distinct facts: on the GameOver arm the eliminated set is exactly the victims; on the Abort arm it is empty because the crossing cycle was refused, not because nobody crossed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): correct four doc-accuracy claims and re-cite CR 101.2 in the bounded-offer rows Fix round 3 of the combo-detector player-feedback review loop. Doc-only: zero production or test-behaviour change; all five findings were LOW. - LOW-1: the flip-count sentence mixed epochs (numerator pre-commit, denominator post-commit). Now states 1-of-83 as measured on the pre-row tree and 2-of-85 on this one, with the runner and filter named. - LOW-2: CR 119.8 governs life exchanges, redistribution, and pay-life costs -- not a "Your life total can't change." override. Re-cited to CR 101.2, which is the engine's own convention and what the sibling fixture doc already names. - LOW-3: a bare "(4167)" suite count with no runner and no filter is deleted rather than re-dressed, mirroring the same removal in the unit module's doc. Swept both files; it was the last one. - LOW-4: the basis-A drive row now scopes itself as synthetic-fixture-only -- every real 4p dump in the file certifies on basis B. - LOW-5: "basis A refuses a mill ring outright" rested on evidence measuring only the equal disjunct. Measured the cover disjunct too: cover == false at all three ring indices, so the claim holds -- and the probe corrects the reason at the oldest frame, which is board-EQUAL and refused by net_progress_for instead. Verified: cargo fmt --all; cargo clippy --all-targets -- -D warnings (exit 0); cargo test -p engine --lib -- game::engine::bounded_offer_conjunct_tests:: (4 passed / 0 failed, 17870 filtered out); cargo test -p engine --test integration -- loop_shortcut:: (85 passed / 0 failed, 4090 filtered out). Assisted-by: ClaudeCode:claude-opus-4.8 * docs(engine): fix the CLAIM, not the site -- four sibling doc corrections Fix round 4 of the combo-detector player-feedback review loop. Doc-only: zero non-comment lines on either side of the diff; all four findings were LOW. Every one was a SIBLING SITE that round 3's fix did not reach, or self-contamination that round 3's fix introduced. - LOW-1: drain_ring's doc -- the DEFINITIONAL site a reader reaches first -- still carried the blanket "a mill ring's frames are board-UNEQUAL" that round 3 corrected 160 lines later. Now states the per-index truth: board-unequal at every index EXCEPT the oldest, which pops zero cards, is board-EQUAL, and is refused by net_progress_for on its zero delta. The conclusion (basis A certifies nothing on a mill ring) is unchanged and still measured. - LOW-2: "this is the only row that detects a back-door deletion of basis B" was measurably false and licensed exactly the overread the round-3 SCOPE note exists to prevent, from 870 lines earlier. Re-measured with the file's own prescribed attribution probe (ring_delta_signature -> None unconditionally), runner cargo test -p engine --test integration -- loop_shortcut:: : 74 passed / 11 failed / 4090 filtered, against a clean 85 passed / 0 failed. ELEVEN rows flip; all ten others are now named so the claim is checkable. - LOW-3: the file named three rules for one const -- CR 101.2 twice and a governing CR 119.8 once. The outlier survived the round that fixed its twin. Both life-loss-immune fixture docs now read "CR 101.2 ... cf. CR 119.8, which governs only life EXCHANGES, REDISTRIBUTION and pay-life COSTS". Swept both files: no governing 119.8 remains. - LOW-4: round 3's own correction became the file's first in-comment test attribute, so its stated counting recipe (grep the attribute over this file) returned 86 while the runner reported 85 -- the fix for a counting error broke the count by containing the thing being counted. The denominator is now anchored to the runner, which is where the number came from, and the quoted literal is gone (grep and runner agree at 85 again). Verified on the frozen final tree: cargo fmt --all; cargo clippy --all-targets -- -D warnings (exit 0, zero warnings) -- the gate the round-4 reviewer honestly did not run; cargo test -p engine --test integration -- loop_shortcut:: (85 passed / 0 failed, 4090 filtered out). Attribution probe reverted byte-identically (md5 match) before any edit landed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): scope two universals in the basis-B control's doc to their measured sets Fix round 5 of the combo-detector player-feedback review loop. Doc-only: 26 added and 4 removed lines, all `///`; zero non-comment lines on either side. Both LOW findings were universal quantifiers that had never been checked against the set they quantify over -- one of them introduced by round 4's own replacement sentence. - LOW-1: "each of those ten fails for a reason its own doc does not name" is false for the first row it names. Now NINE of ten, with dina called out as the exception: its CERTIFICATION BASIS note already documents this very probe as measurement (ii), down to the 400-beat cap and the `expect` firing. Measured by scanning all ten doc blocks for `ring_delta_signature`/`basis B` (dina 6 hits, the other nine 0), plus a second pass for probe-adjacent wording. - LOW-2: annotating dina alone as "(the real 4p dump)" implied an exclusivity it does not have, and contradicted the plural "the file's real 4p dumps" 900 lines later. Dropped, and replaced by the measured split over the whole set: SIX of the eleven load the real dina_conqueror_4p capture, five are GameScenario builds, and bounded_fixed_count_commits_exactly_n_periods is the one MIXED row. Classified by resolving every fixture-loading call site to its enclosing test fn. The count basis is stated in the doc as call-site resolution, NOT grep hit count, because these sentences themselves add doc-comment hits for the names they list -- the self-contamination class this lane earned in round 4. Both measurement methods were re-run after the edit and returned the same numbers. Verification: cargo clippy --all-targets -- -D warnings, exit 0, zero warning or error lines. cargo fmt --all clean. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): point our repro commands at the renamed phase-engine package Upstream 2b204dff5 (#6739) renamed the engine package to phase-engine, so the six `cargo test -p engine` repro commands in these two files' doc comments no longer resolve. Comment-only; no non-doc lines changed. Upstream's own stale `-p engine` doc comments in eight other files are left alone -- provenance-checked against upstream/main, they are not ours. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): route player choice through one legality authority (5c) Every site that materializes a player choice or validates a player target now goes through a single authority instead of ad hoc per-site filters, so a seat that has left the game or phased out can no longer be offered or pinned. - CR 102.1: eliminated players are not choosable or targetable. - CR 702.26b: a phased-out permanent's controller is treated as absent for choice and target legality. - CR 608.2b: pinned player targets are re-validated mid-drive; an unresolvable pin withdraws the offer rather than publishing an undeclarable point. - CR 732.2a: a wire-encoded zero-repetition shortcut bound is rejected at the load seam, closing a window that let a zero-cycle proposal deserialize. Also repairs the save-compat path: dump loaders now decode through the production PersistedGameState decoder rather than bypassing it, and fixture migration is re-runnable from the read-only pristine root via scripts/migrate-dump-fixture.sh. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): split LoopDetectSample into normalized and live ring halves (5d U0) CR 104.4b: the normalized half stays the draw-detection comparand; the live half preserves what normalization erases so later 5d steps can evaluate period touches against real board state. Ring re-typed VecDeque<Arc<LoopDetectSample>>, serde(skip) unchanged (zero persistence surface), normalize_for_loop body untouched. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): event-derived resolution obligation and prompt-cause partition (5d U1) CR 616.1/614.1a: replacement-prompt causes derived from the proposed-event stream (ReplacementPromptCauses bit-set over find_applicable_replacements); 29-variant wildcard-free event_is_accounted partition; probe_resolution with the four Prompted arms; bind_resolution_scope extracted from resolve_top (CR 608.2k); UpTo guards on all six promoted quantity arms; forward-split verdict_memo::ProbeBudget (narrowed at U3). Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): shape-B mint conjuncts and declare-time owner firewall (5d U2) CR 603.5/732.2a: entry_publishes_pin_slots gains the recipient, stored-auto-choice, and mint-level cardinality conjuncts for both mint shapes; declare-time owner firewall validates against LoopShortcutOffer.proposer with apply_confirmed_shortcut re-validation; firewall placement covers the empty-schema route (R28 a-double-prime pins it). Known pre-existing defect disclosed, not fixed here: ShortcutProposal.per_cycle PlayerId-keyed map fails JSON key deserialization, making persisted bounded-shortcut saves unloadable. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): re-derive the loop-shortcut probe budget from the offering beat Measured through `try_offer_bounded_cycle_shortcut_metered`, three real 4p dumps driven through `apply()` at an unbounded cap: dina beat 19 spent=13 asks=13 skips=0 ring=3 stack=10 OFFER dina beat 45 spent=73 OFFER (post-fast-forward) dellian max spent=107 asks=5 ring=16 stack=177 no offer in 200 beats f4 max spent=0 no offer in 200 beats R16(ii-a): demand at the corpus's acceptance beat is 13, so the shipped cap of 12 starved it by exactly one charge. PROBE_BUDGET 12 -> 26 (2x the measured demand; the unexempted sweep measures 96-107, so an unexempted classification still exhausts and still refuses fail-closed). R16(ii-b), recorded: the max spend across ALL mintable beats is 107, at non-offering dellian beats where refusal is correct. BOTH AXES at the offering beat, per the R33 escalation: basis = B (`ResourceSignatureOnly`); the within-basis-A disjunct has NO VALUE — basis A certified 0 times across all three dumps (129 basis-B certifications). Named-dump caveat: dellian and F4 reached no offering beat in 200 beats under `dump_drive_one_beat`, so the witness is dina. Consequence for the cost table (§3 D4.3): the frozen exemption's speed-up applies at dellian-shaped beats, which exist in-corpus but never offer under the shipped driver. THE OFFERING BEAT PAYS THE FULL UNEXEMPTED SWEEP — `skips=0` there — so the 854x headline no longer describes the beat this class actually offers on. `MintMeter` gains `certification`, the only surface on which the certifying disjunct is observable: both bases publish `frames_per_period`, so the published `LoopCertificate` discriminates in neither direction. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): pin the frozen exemption to its certifying disjunct (5d U3, R33) Six arms on the CR 732.2a bounded-offer frozen exemption, all on real-dump or constructed-equality windows driven through `apply()`: - (a)/(b)/(a'1)/(c1)/(c2) on the real dellian window — beat SEARCHED by construction requirements, landing on beat 14 / 152 frozen, independently agreeing with the plan's RF13-BEAT row; (a) bounded by an independent longest-common-prefix scan, never f(x)==f(x). - (a'2) at the SELECTION site on a constructed equality-certified ring, with both reach-guards firing (non-empty BoardCovered frozen set; certification == Some(BoardEqualOnly)). - (d) landed previously (060d0cf64). Revert-probes, all run and restored byte-identical: 1. delete the pre-walk early return => (b) flips, 152 frozen leak back 2. basis-B call site -> BoardCovered => (d) flips 3. delete step 4b (round-39 shape) => (a'2) ALONE flips while a/b/a'1/c/(d) all still pass — the round-40-vs-round-39 discriminator this arm exists for. R33(c2) keys to the PROBE_BUDGET constant (12->26 needed no edit); the 96-107 unexempted band is recorded, not asserted. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): budget-refusal and warmup-meter rows for the bounded offer (5d U3, R15/R16(v)/R20) - R15: the real dina offer beat replayed at ProbeCap::Lowered(0) refuses with UnspecifiedChoiceWindow while Shipped offers; probe (delete probe_resolution's try_charge_one arm) FLIPPED — the zero-cap board offers at spent:0 asks:13, D=13 confirmed by a third independent instrument. Basis B certifies for free at any cap (no board predicate); the denial is conjunct (6)'s. - R16(v): a ring-starved dellian beat at priority (stack > 2) yields NoCertification with (spent, denied, asks, scans) == (0,false,0,0). DISCLOSED LIMITATION in the test doc: MintMeter is snapshotted below the ring gate's early return, so a hoisted eager pass is invisible to this instrument — the observable half is pinned; the structural half rests on construction order. - R20: 27 entries (PROBE_BUDGET + 1) with the last CHAINED so the bound is per-link; the same board offers at RaisedTwiceLinks, refuses at Shipped with denied at spent == PROBE_BUDGET, and a pre-charge NotAtPriority control leaves a clean meter. Probe run twice: run 1 masked by (i)'s spent >= entries clause (re-keyed to conjunct6_asks — what the gate examined, not what it charged); run 2 flips in the plan's direction (over-budget board OFFERS, asks:27 spent:0). - Shared fixtures extracted (dina_driven_to_bounded_offer, replay_at_priority, equality_ring_with_stack); R33 arm (a'2) rewired to the helper and re-run green. All probes restored byte-identical; resolution_prompt.rs carries zero diff. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): offer-writer census and carrying-frame rows (5d U3, R8/R14/R19/R29/R31/R32) Purely additive test rows on top of 93dc52c02; six revert-probes run, flipped, and restored byte-identical. Suite arithmetic pins the delta: lib 18144 -> 18149, integration 4311 -> 4313. Three plan contradictions disclosed, none silently absorbed: - R8's ROUND-7 pre-change check is comment-blind: U2's doc comment at engine.rs:4168 makes a bare anchor measure 23/12. The census excludes comment lines (a comment writes no offer and consumes none), restoring the plan's 22 production / 12 test and its per-file production multiset exactly. - R29's stated (c) probe had no flipping site as written ((c1)/(c2) sit on the predicate, the probe edits an argument they never traverse); ADDED arm (c3) driving the same crossing through stack_choices_are_all_specified — the probe flips (c3) alone. - R21 arm (a3) is FALSIFIED and not written: it demands frozen_skips > 0 at the offering beat, but the corpus's offering beat certifies through basis B with skips == 0 — the exact fact R33 arm (d) pins. A row contradicting a shipped green row does not ship. CR anchors: 115.2, 601.2c, 603.3c, 603.5, 608.1, 732.2a — all re-read against the code they describe. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): verdict-door totality, keying and proposer rows (5d U3, R22) All five R22 conjuncts across four tests so each conjunct's revert-probe lands on its own assertion: totality/frame/proposer (1)+(2)+(2'), the window-offset- zero vacuity boundary (green under all four probes, never counted as coverage), conjunct (3) foreign-frame refusal, conjunct (4) effective-key proposer. Four probes run, flipped, restored byte-identical (0 deleted lines in the diff): id-only memo key; unchecked frame_ix; hard-coded proposer; deleted relief agreement guard. The fourth flipped only on run 2 — run 1 exposed a VACUOUS relief negative: entry_publishes_pin_slots early-returns on entry.controller != proposer (engine.rs:1670), so the B-bound container answered None upstream of the guard under test. Fixed with a B-controlled entry plus a positive reach-guard (under B's own pins the entry IS relieved); the probe then flipped. CR 603.5 anchor re-verified. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): close the U3 remainder — R16, R17, R21(b)-family, R27(a2), Arc::as_ptr Nine test-only rows, +919/-0, no production line touched. MEASURED, through `try_offer_bounded_cycle_shortcut_metered` and through the predicates the mint calls, on both TRACKED 4p dumps driven via `apply()`: dina beat 19 demand=13 offer fires, cap does not bind (D searched, not copied) dina beat 10 stack=8 announced=3 frozen=5 asks=6 skips=5 sum=11 dellian beat 14 stack=154 announced=2 frozen=152 asks=4 skips=152 sum=156 dellian beat 14 mint: NoCertification, spent=26=cap, denied, scans=36, 1.79 s (debug) id freshness: dellian 227 announcements / 50 resolutions / 0 revivals; dina 88/17/0 R21(b)'s SUM IDENTITY is CORRECTED, not copied: the plan's "sum = current.stack.len() = 154-156" is HEAD-era. Post-U3 the domain is `announced ∪ (stack \ frozen)`, so the measured sum is `announced + stack` (156 and 11). The plan's asks/skips figures themselves reproduce. R21(b-placement-B)'s stated matched pair is FALSIFIED and re-keyed: the unmutated dellian beat-14 board does NOT offer (item (4) already trips at stack index 35, and that entry is ITSELF frozen), so no mutation is needed — item (4) scans 36 entries where only 2 are non-exempt, while conjunct (6) on the same touch skips all 152. R27(d) is NOT WRITTEN, measured vacuous four ways, including running the plan's own stated revert (comparand ring <- .live): 4/4 green, nothing flips. Basis A certifies 0 times corpus-wide; `frames_per_period`/`delta` come from `ring_delta_signature`, which reads `f.normalized` directly from the sample and never consults the mint's `ring` vec; `residual_board_delta` is byte-identical across the halves. R16(ii-a)'s `spent <= PROBE_BUDGET` is VACUOUS by construction (a budget cannot overspend), so the row ships the denial flag plus an exact-demand sweep across the seam's closed cap domain: `Lowered(13)` offers, every `Lowered(n < 13)` refuses. R16(iv)'s ceiling is DEBUG-scaled and says so. Eight revert-probes applied, seven flipped, all restored byte-identically. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): retained-sample .live-reader rows close U3 (5d, R27 a3/b/c/e) Four arms on a CONSTRUCTED board driven through the real mint, where ring frame 2 carries an entry neither frame 1 nor `current` holds — so the announced pair genuinely arrives from a retained sample and the shared carrier revert (ring_live <= .normalized) has a BEHAVIOURAL flipping site, defeating the round-7 blind-spot tripwire for (b)/(c): - (a3) retained sample's Effect::Token derivation equals the live board's; control shows the .normalized half derives a different set. Flips on P-A (clear_trigger_identity_recursive source_id scrub deleted): (940,940) vs (940,0). No mint-level flip — measured and disclosed in-code (both derivations are event_is_accounted). - (b) stored MayChoice auto-record survives the ring: no record => OFFER publishing exactly 1 MayChoice point; record seeded => UnspecifiedChoiceWindow. Flips on P-B (carrier revert): the negative OFFERS. - (c) intervening-if binds with the retained sample's trigger source; the "classifies identically to current" conjunct and a MayPrompt control on the normalized half ride the same row. Flips on P-B: the positive REFUSES. - (e) discharge reads the pair's carrying frame, not the live board — four boards (frame-only / both / neither / causeless) plus the frame_ix pointer-identity structural conjunct. Flips on P-C (resolution_events_are_discharged frame => state), both-boards arm green in the same run. R8 census tripwire fired benignly: (22,12) => (22,13) — production half unchanged, per-file multiset unchanged; the +1 is (b)'s cfg(test) read of the offer schema, adjudicated per the row's protocol in its doc and failure message. All probes restored byte-identically (sha-verified). CR gate: 12 numbers, 0 unverified. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): OptionalEffectChoice pin-injection arm with seat and beat guards (5d U4) inject_pinned_answer gains the CR 603.5 MayChoice consumption arm (plan section 3 D5 verbatim) with both total head guards in source order — seat (*player != template.owner => RecastAbort, CR 603.5) and beat (work.pending_trigger.is_some() => RecastAbort, CR 603.3c announcement-time question is never answerable by a resolution-time pin) — plus the two injector doc corrections the arm falsifies (Targets-pin/MayChoice-pin split; the _ arm's remainder is "(mode / unless / X)"). Rows: R23(4) seat matched pair; R23(5) beat matched pair on the SAME source_id (differing-source would be refused by the slot lookup and report the wrong guard); R28(b) re-keyed on measurement — the plan's "drive seam must still RecastAbort for a matching hostile owner" is FALSE and its own cell says why (the comparand is client input): (b1) asserts the breach loudly with the re-key instruction in-code, (b2) asserts the declare-seam refusal that makes the ingress unreachable, matched positive included. Fourth row added for the accept mapping (both MayChoiceOption directions — live inverted-mapping failure mode with no plan row). R23's (5-reach) ships with R2 in U5. Four revert-probes run, flipped, restored sha-identical. P3 (declare firewall deleted) flips (b2) printing the tampered proposal reaching APNAP; (b1) is a measured must-not-flip control; U2's r28_a/r28_a'' flip on it too, r28_c stays green. Duplicate-needle tripwire fired once on P4's first application (doc quoted the production expression verbatim); doc rewritten, needle re-measured 1, probe re-run and flipped. Census adjudications, never relaxed: CR 603.5 prompt census 34=>37 (producer half unchanged at 5); R8 offer-writer test half 13=>14 (production unchanged at 22, multiset byte-identical) — new sites named in doc + failure message. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): track the F4 4p dump and pin the bounded-offer behaviour it actually has Stages `crates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gz` for the first time, together with the integration module that loads it through the production chokepoint (`PersistedGameState::into_game_state`) and drives it beat-by-beat through the public `apply()` — no synthetic `GameScenario` stands in for the real board. Rows: R18 (name-resolver fails loud both ways), R1 + R1b (offer fires, bound re-derived independently of `elimination_bounds`, published point set), R2 (what an accepted declaration commits), R23 5-reach (no answered may-beat carries a construction cursor), R9 (refusal keyed to a derived replacement obligation, not a definition name), R16 (exact probe demand at the offering beat), R27 a1 (the recorded sample keeps a live half normalisation would erase), and R7 in `analysis/resource.rs` (the frozen prefix is an index-id identity, never a presence count). MEASURED CORRECTIONS, pinned rather than asserted-as-planned: * The bounded offer FIRES on the real dump (beat 43, P0, max_iterations = 35, basis B) but publishes ONE decision point, not the three §6 R1 predicted, and an accepted declaration therefore commits ZERO cycles at both n = 1 and n = 3 — it answers Sue's `may` from the pin and then aborts on Reed's unpinned `may`. Fail-CLOSED (rollback, CR 800.4a handback): nothing rules-wrong ships, but it is not a grant. R1/R1b are labelled PARTIAL and §6 R2a/R2b/R3/R4/R5 plus the interruptibility matched pair are deliberately NOT written — no row asserts a falsified prediction. * CONSEQUENCE-DISCLOSURE of the ratified `PROBE_BUDGET = 26`: §6 D1/D2 (dellian) are unreachable and are NOT written. dellian drives 309 beats through `apply()` to `GameOver` with no offer ever raised; every mintable beat refuses `UnspecifiedChoiceWindow` with `spent = 26 = PROBE_BUDGET, denied = true`. dellian's measured demand at that seam was 96–107, so this is budget exhaustion by design of the cap, not a detector fault. Remedy sizing for the follow-up is journaled with measurements (widening `announced` is insufficient alone; the sampler gate is not the seam — two relaxations left the frame census unchanged; the resolution-order sequence is empty at the offering beat). Assisted-by: ClaudeCode:claude-opus-5 * test(engine): pin what the AI can actually do at the real F4 bounded offer §5 U6's AI-verification step, keyed to the measured reality rather than to the plan's three-slot prediction. Drives the tracked 4p F4 dump to its real CR 732.2a offer through `apply()` and measures `engine::ai_support::legal_actions` there — the seam `phase-ai/src/search.rs` reaches via `WaitingFor::LoopShortcut { .. } => engine::ai_support::legal_actions(state)`. MEASURED: the generator emits exactly two candidates, `DeclareShortcut { count: UntilLethal, template: None }` and `DeclineShortcut`. Its `Fixed(max_iterations)` candidate — which exists precisely for bounded offers — is gated on `schema.points.is_empty()`, and this offer publishes one point, so it is excluded; the surviving declare candidate is refused outright by `handle_declare_shortcut` (`UntilLethal` against a narrowed bound). The AI's only effective action is therefore DECLINE, which the phase-ai policy independently reaches: the offer latches `predicted_winner: None`, routing `LoopShortcutPolicy` to its `(None, UntilLethal) => reject` arm. These rows supply the reachability that policy's own row cannot — a real captured board carrying exactly that pair. The whole declare option space is measured on one board: `UntilLethal` + None, `UntilLethal` + a conformant template, and `Fixed(max)` + None are all refused into the CR 800.4a handback; `Fixed(n)` + a template pinning every published point with `owner == proposer` is ACCEPTED and opens the CR 732.2b window — the anti-vacuity control, and the shape the generator never emits. A third row exercises the declare-time `template.owner` firewall on the real dump (the staged-offer arm lives in `loop_shortcut.rs`'s `r28_a`). THE GAP IS REPORTED, NOT CLOSED, for two measured reasons: an accepted declaration on this board commits ZERO cycles (the unannounced-`may` defect pinned by `r2`), so a candidate-generator fix rides the grant mechanism and inherits its hold; and building the template needs a new engine authority for the pin CONTENT, which is a design decision rather than executor-local plumbing. §6 R8's U6 invariance arm — the one arm §5 leaves open — PASSES: U6 touches no tree R8 walks (`git diff --stat HEAD -- crates/engine/src crates/phase-ai/src` is empty), the raw `WaitingFor::LoopShortcut {` census is unchanged at 37, and R8's own tests stay green at `(production, test) == (22, 14)` with the `ai_support/candidates.rs 1` multiset entry intact. No census update is owed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): review-impl LOW fixes — board-not-prompt contract, real Braids text, census pin Three LOW findings from the independent full-diff review, applied without any logic change: - The three-field "pure over (stack, objects, proposer)" wording at the mint's three sites is replaced with the board-not-prompt contract the U2 doc already states (the mint reaches eleven GameState fields through optional_prompt_player; what callers rely on is PROMPT-independence). The CR 603.3c relief justification is restated on the stronger premise: the pending_trigger_entry cursor is prompt-coupled by every production writer. - The Braids, Conjurer Adept quotes at three sites now carry the card's real Oracle text (verified against Scryfall twice, executor and reviewer independently) with the fixture's Effect::PutCounter named as a synthetic stand-in for the class, sound because the branch is effect-agnostic. - .combofb-5d-cr-gate.md gains a committed-range reconciliation: 49 distinct CR numbers on added lines, 0 unresolved, +7 previously untabled, 2 stale rows superseded; two independent instruments agree and a fake-number positive control proves discrimination. One knock-on adjudicated per the census test's own protocol: the +7 net doc lines above engine.rs:10493 moved the CR 603.5 producer to :10500; the pinned string is re-adjudicated with the cause named (producer byte-identical; the total-37 and 5/7/25 partition asserts fired green in the same red run, proving the set never moved). Near-miss recorded in the module doc: an adjudication doc must never quote the census needle verbatim — it would self-count. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): derive CR 603.7 firing carriers for the 4p dump corpus Upstream #6842 (8121fd1c6) made a `TriggerFiring` carrier MANDATORY on every persisted triggered record and fails CLOSED without one. The six 4p dump fixtures this branch drives were captured before that commit, so on the rebased base they no longer decode at all: 41 of the 44 red rows were production-decoder rejections naming the three carriers (`active legacy pending trigger has no firing discriminator`, `triggered stack entry has no firing carrier`, `resolving triggered entry has no firing carrier`). The carrier cannot be RECOVERED, only DERIVED. The read-only pristine root (`combofb-dumps-pristine/`, captured 2026-07-22/25) predates #6842 too and records zero firing carriers, so re-deriving from pristine cannot mint a value that was never captured. `TriggerFiring::UnknownLegacy` is not an escape hatch, and this was measured rather than assumed: `validate_firing` (7 call sites, types/game_state.rs :7688/:7691/:7700/:7721/:7732/:7749/:7779) returns `Err("... has no canonical trigger firing discriminator")` for it. It is the field-absent marker (`skip_serializing_if`) and the redaction default, never a legal persisted value. THE DISCRIMINANT (CR 603.1 ordinary vs CR 603.7a delayed), applied per record: Ordinary <= the fired trigger's definition is present on its SOURCE OBJECT's own `trigger_definitions`/`base_trigger_definitions`, matched by exact `description`. A printed or granted triggered ability of a permanent is an ordinary triggered ability. Delayed <= the trigger has an install receipt in `delayed_triggers`. Anything else ABORTS by name. There is deliberately no fallback stamp: a wrong carrier silently re-classifies a CR 603.7 firing identity, which is precisely the inference upstream refuses to make. Measured, per fixture — carriers stamped, and `delayed_triggers` length: | fixture | carriers | delayed_triggers | sha256 (stamped) | |---|---|---|---| | dellian_emblem_conqueror_4p | 154 | 0 | 3f8b06eb7986de10 | | dina_conqueror_4p | 7 | 0 | 773e902daccf7199 | | fantastic_four_bounded_loop_4p | 3 | 0 | 89ff377b53b7bd55 | | tenacity_exquisite_blood_4p | 6 | 0 | a296ab6e37129ca3 | | witherbloom_sprout_lumaret_4p | 1 | 0 | 71efd26867e9fc57 | | witherbloom_sprout_lumaret_simple_4p | 1 | 0 | 607fbe632e759692 | 172 carriers total, ALL `Ordinary`, ZERO undetermined. Every fixture records `delayed_triggers: []` and no install journal, so `Delayed(Some(prov))` could not have validated regardless — `validate_firing` demands a registered install root. Every one of the 172 was classified from its own record's source object, never defaulted; the per-record witness table (carrier, source id, card name) is in the PR body. Five resolving entries elsewhere in the corpus are NOT migration targets and are deliberately left unstamped: `combo_infinite_pile_4p_untapped_precast`, `kilo_freed_relic_pentad_4p`, `sprout_witherbloom_realistic_lands_4p`, `basalt_power_artifact_infinite_colorless`, `combo_infinite_pile_4p_offer`. Each `kind.type` is `Spell` or `ActivatedAbility`, not `TriggeredAbility`, and the validator's `(Some(_), None) => {}` arm accepts a non-triggered resolving entry with no carrier. An earlier classifier of mine flagged these as UNDETERMINED; reading each entry's `kind.type` showed the flag was wrong. SECOND FIELD CLASS — the CR 603.7 delayed-trigger ALLOCATORS. Restoring the dumps exposed a further #6842 change that the decode failures had been masking: `next_delayed_trigger_token` carries `#[serde(default)]`, so a pre-#6842 dump that omits it restores as 0 through a bare `GameState` decode, while the production `PersistedGameState` path runs a load-time repair (`next = max(existing // 1, max_used_token + 1)`) and restores 1. The two decoders therefore disagree on a legacy dump, and 0 is invalid on its face: `validate_trigger_firing_coherence` rejects `next_delayed_trigger_token <= max_token`, which is 0 when there are no install roots. This surfaced as the R8 state-neutrality arm of `migrated_dump_decodes_through_both_decoders_and_unmigrated_through_neither` reporting `differing paths: ["state.next_delayed_trigger_token"]`. That arm is NOT relaxed. The fixtures are stamped with the repaired value instead, which makes them look like a modern capture, keeps both decoders in agreement, and survives the eventual deletion of the load-time shim. Measured, all six fixtures: field absent, `delayed_triggers: 0`, and ZERO `DelayedTriggerInstall` commands across 158/30/2/516/50/5094 journal entries. So both used-token sets are empty and the formula collapses to `max(1, 1) = 1`. The scan is not vacuous: injecting one synthetic install command into a fixture makes the same selector report 1 instead of 0. The GENERAL derivation of the used-token set is deliberately NOT reimplemented in jq — it walks `resolved_rules_journal` install commands and `delayed_triggers` provenance with reuse and nonzero checks, and re-deriving it here would repeat exactly the mistake this script's sibling refuses to make for `EffectKind`. Only the collapsed no-install-roots case is stamped; anything else ABORTS BY NAME, verified by probe (one injected install command => "UNDETERMINED delayed-trigger allocators: 1 install command(s) ...", nothing written, exit 1). MECHANISM. `scripts/lib/trigger-firing.jq` holds the SINGLE definition of the derivation; both the pristine-regeneration path (`migrate-dump-fixture.sh`) and the in-place path (`stamp-fixture-firing.sh`) load that one file, so neither can certify its own copy of the recipe. WHY IN PLACE RATHER THAN A PRISTINE REGENERATION for these six. Regeneration is the stronger provenance and is preferred where it applies, but it does not apply here: measured, `dina_conqueror_4p` and `witherbloom_sprout_lumaret_simple_4p` differ from their pristine regeneration in exactly one object each (Priest of Forgotten Gods' `abilities`/`base_abilities` AST), because the committed fixture carries a LATER parser state than the capture. Regenerating them would silently REVERT that. Stamping in place is additive and cannot revert anything. ALL THREE control arms are enforced, and the stamper refuses to write if any fails: arm 1 NO_COLLATERAL the stamped artifact minus the five stamped keys is BYTE-IDENTICAL to what was committed, so nothing but the carriers moved. arm 2 CARRIERS_ADDED got == need && got > 0, keyed on CARRIER COUNT rather than on byte difference. An earlier revision of this arm keyed on bytes and reported a false pass for zero-carrier fixtures, where gzip/jq re-serialization alone changes bytes without stamping anything. Fixtures needing zero carriers are now SKIPped outright. arm 3 ALLOCATORS_CANONICAL both allocators exist and are >= 1 — i.e. above the value the engine's own coherence validator rejects. Also fixes a pre-existing defect in `migrate-dump-fixture.sh` that the corpus sweep exposed: the `target_slots` stage used an unguarded `|=`, which aborts with "Cannot iterate over null" on a dump paused at a beat with no target prompt. The script was therefore only ever usable on 2 of the 6 dumps. The stage is now guarded, so ONE recipe covers the whole corpus. The `gameState`-envelope guard in the jq library is the same class of fix, caught by the control before any write: without it, `.gameState |= ...` would have CREATED a `gameState` key on the four `turn_number`-enveloped dumps, i.e. corrupted them. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-anchor the CR 603.5 prompt census after the rebase onto #6842 The census tripwire pins the exact source coordinates of every CR 603.5 prompt producer so that a SIXTH producer is a counted event rather than a silent one. Rebasing this branch onto upstream #6842 (8121fd1c6) moved five of those coordinates, and the row fired. ADJUDICATED, NOT RELAXED. The row fired on COORDINATES, not on population. The producer count is still 5, not 6: game/effects/mod.rs :5896 :5973 :8927 => :5918 :5995 :8949 game/engine.rs :10500 => :10589 game/effects/scoped_library_search.rs :452 => :452 (UNMOVED) `mod.rs` is a uniform +22 shift and `engine.rs` a +89 shift — the signature of lines inserted ABOVE each site, not of a new producer. That `scoped_library_search.rs:452` did NOT move is itself evidence the SET did not change: a genuinely new producer would not leave an untouched file's coordinate fixed while shifting the others by a constant. The producer TEXT at all five sites was verified byte-identical between the pre-rebase tree (`chain3-prefold-backup`, dbc81821d) at the old coordinates and this tree at the new ones. So the expectation array is re-anchored to the new coordinates and the assertion keeps its full strength: a sixth producer still fails this row. Correcting my own earlier report for the record: I previously stated that upstream had ADDED a CR 603.5 producer. That was wrong — the count never left 5, and the failure was purely positional. The correction is carried in the tripwire's own doc block so the next reader of this row does not inherit the mistake. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-anchor the CR 603.5 prompt census after the rebase onto #6851 CI built the MERGE ref (branch + main 96e41b3ab) while the branch was still based on e12447f4f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by #6851's own insertions above this producer: `git diff -U0 e12447f4f 96e41b3ab` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and #6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): close two fail-open seams in the loop-shortcut analysis proofs Addresses maintainer Critical 1 and Medium 6 on #6886, plus CodeRabbit 3699361027 / 3699361052 / 3699361032, all in `analysis/resource.rs`. 1. CRITICAL — `ShortcutProposal.per_cycle` could not survive the PRODUCTION persistence path. Adds a serde adaptor riding the four `PlayerId`-keyed `ResourceVector` maps as pair SEQUENCES, generalizing the existing `counter_key_pairs` into `map_key_pairs` so one definition covers the tuple key and the player keys. MEASURED MECHANISM (the old doc comment asserted the opposite, and was wrong): a bare `BTreeMap<PlayerId, i64>` is fine through `from_str` and through `from_value` IN ISOLATION — which is why the existing `periodic_delta_survives_the_serde_json_wire` arm passed and gave false confidence. It breaks only under the ENCLOSING shape: `WaitingFor` is `#[serde(tag, content)]`, so its payload is buffered through serde's `Content`, which stringifies map keys, and `PlayerId` is `#[serde(transparent)]` over `u8`. `PersistedGameState::deserialize` routes EVERY decode through `serde_json::Value` + `from_value`, including the WASM restore at `engine-wasm/src/lib.rs`'s `from_str::<PersistedGameState>` — so `from_str` at the boundary does not save it. Measured on serde_json 1.0.149; the failure text is `invalid type: string "0", expected u8`, exactly what `tests/integration/loop_shortcut.rs` had already recorded as a standing limitation. `generic_triggers` keeps its bare map: `TriggerKind` is a unit-variant enum, measured Ok through the same path. NEW ROW `a_populated_per_cycle_proposal_survives_the_production_persistence_ boundary` drives `from_value`, the `PersistedGameState` boundary, and the WASM bridge's own `from_str::<PersistedGameState>`, with all four `PlayerId`-keyed maps populated behind a reach-guard. REVERT-PROBE, run: dropping `#[serde(with = "map_key_pairs")]` from `life` ⇒ FAILS with that exact error text; restored byte-identical. 2. MEDIUM — an EMPTY `FreeUnlessReplacements` derivation discharged the CR 616.1 obligation vacuously. `!events.iter().any(..)` is `true` for an empty slice, and the only thing preventing it was a `debug_assert!`, which compiles out of release — so the fail-open case was live in the build that ships. Now a first-class refusal in every build. A refusal rather than a panic on purpose: it matches every other seam in the module, and a `debug_assert!` could not be covered at all, since it aborts the build tests run in. REVERT-PROBE, run: deleting the arm ⇒ the empty case flips to `true` and the new row FAILS while the non-empty arms stay green; restored byte-identical. 3. Comment defect (CodeRabbit 3699361032): the zero-delta early return claimed "every longer period is a whole number of copies of this one". The repetition test inspects only the most recent `2k` deltas, so a larger `k'` need not be a multiple of `k`. Behaviour is unchanged and still fail-closed; the false justification is corrected in place, with the counter-example, because it is the kind of claim a later reader would lean on to widen the search while keeping the early return. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): refuse the resolution probe when a prompt already stands Maintainer Critical 2 on #6886, and CodeRabbit 3699361064. `probe_resolution` compared only the `WaitingFor` DISCRIMINANT of the probed clone against the incoming board. When the incoming board already carries a non-priority variant, a resolution that re-parks the SAME variant leaves the two discriminants equal, so the probe reported the resolution CHOICE-FREE while an unanswered choice sat on the board. That is fail-open in the one direction this function exists to close, and no comparison against the incoming variant can see it — the incoming variant is exactly what masks it. Now keyed on "is there a prompt at all": a non-`Priority` `waiting_for` on the probed board is itself a refusal. The incoming board is a RESOLUTION BOARD by this function's own documented contract, so a standing prompt is a reason to refuse rather than a baseline to compare against. The discriminant test is kept alongside it, so the guard is STRICTLY STRONGER on every input than the struck form: it can only ever cost coverage (a missed offer), never soundness — the same direction as the budget-exceeded and empty-derivation arms beside it. NEW ROW `a_prompt_standing_on_the_incoming_board_refuses_the_probe`, a MATCHED PAIR over all six allow-listed arms: each arm must still reach `Events` from a priority board (positive control — without it a probe that refused everything would pass), and must return `Prompted` when the same board carries a standing `ReplacementChoice`. The row asserts the resolution does NOT clear that prompt, so the discriminants really are equal and the struck guard could not have caught it. REVERT-PROBE, run: restoring the bare discriminant comparison ⇒ the negative arm FLIPS TO FAIL while the positive control stays green; restored byte-identical. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine,ai): offer only legal quantity choices on a bounded shortcut Maintainer Medium 5 on #6886, and CodeRabbit 3699361023 / 3699361081. The candidate generator emitted `IterationCount::UntilLethal` unconditionally for every `WaitingFor::LoopShortcut` node, including bounded offers. `handle_declare_shortcut` rejects that combination outright (`IterationCount::UntilLethal if offer.schema.is_bounded()` => `reject_shortcut_declaration`, `crates/engine/src/game/engine.rs`), and that reject is a SUCCESSFUL fail-closed handback — `Ok(result)`, not an `Err`. So the candidate was not merely a wasted search node: the simulation layer was handed an action the engine accepts and then silently discards, i.e. an illegal quantity choice wearing the shape of a legal one, which the policy layer then had to know to score away. `UntilLethal` is now gated on `!schema.is_bounded()`. A bounded offer still gets `Fixed(max_iterations)` where its pin set permits a `template: None` declaration; where neither applies, `DeclineShortcut` genuinely is the only legal answer at the node, and representing that honestly is the point. Paired AI-side guard: `LoopShortcutPolicy`'s final bounded arm matched every remaining `Fixed(n)` INCLUDING `n == 0`, so a zero-count declaration — legal and representable, per `a_zero_count_declaration_validates_over_an_empty_range_but_still_checks_ cardinality` — would have been scored into the CRITICAL band. It commits no cycles while spending the CR 732.2b response window, the same weak-domination shape the over-bound and `(None, UntilLethal)` arms already reject. Unreachable from today's generator, so no current ranking moves; the arm now states its own precondition instead of relying on an invariant maintained a crate away. Uses `PolicyVerdict::reject`, not a raw sentinel. R8 OFFER-WRITER CENSUS: unaffected, and checked rather than assumed. The census counts occurrences of the `WaitingFor::LoopShortcut {` token; this change edits the BODY of an existing match arm and adds or removes no such token, so the pinned (22, 14) pair and the per-file production multiset are untouched. Assisted-by: ClaudeCode:claude-opus-5 * fix(scripts): read both trigger-definition shapes and stop overwriting carriers Maintainer Medium 3 on #6886, and CodeRabbit 3699361085 / 3699361087 plus the non-object `command` nitpick. Also folds in the promised doc correction. 1. `_defs` read `.description` across BOTH definition lists, but they do not serialize alike. `trigger_definitions` is `Definitions<TriggerEntry>` and `TriggerEntry` is `{occurrence, definition}`, so its text is at `.definition.description`; only `base_trigger_definitions` (`Vec<TriggerDefinition>`) exposes `.description` directly. MEASURED on the committed corpus, which is what the maintainer asked for and what the earlier "all 172 carriers matched non-null" claim did not establish: of the `trigger_definitions` entries, ZERO expose a direct `.description` and 100% nest it (145 / 165 / 132 on dellian / dina / witherbloom). The live list therefore contributed NOTHING — every entry collapsed to the `// ""` fallback — and all 172 carriers resolved through `base_trigger_definitions` alone. Total descriptions visible to the derivation across the corpus: 875 before, 1755 after. REACHABLE, not theoretical: `dellian_emblem_conqueror_4p` carries a GRANTED trigger ("When ~ dies, you gain 1 life.") present in the live list and absent from the base list. A firing whose description existed only there would have aborted the whole stamp on a classifiable fixture. BEHAVIOUR-PRESERVING on the corpus: 172 carriers resolve before AND after, all `Ordinary`; the pristine regeneration stays BYTE_IDENTICAL=true. 2. `stamp_trigger_firing` assigned all three carrier keys unconditionally without reading them, so `stamp-fixture-firing.sh`'s header claim that in-place stamping "cannot revert anything" was false for exactly those keys — and arm 1 structurally cannot catch it, because it deletes them from both sides before comparing. An already-canonical `Delayed` carrier would either abort the stamp or be silently rewritten to `Ordinary`, the CR 603.7a to CR 603.1 re-classification this library exists to refuse. Now derives only into an ABSENT slot, which also makes the stamp idempotent. Preservation is scoped to stack entries that are still on the stack, so a stale key cannot accumulate and inflate the carrier total past the number of triggered records — the one shape that could have let arm 2's aggregate comparison cancel a surplus against a deficit. 3. `select(.command.DelayedTriggerInstall)` indexes `.command` with a key, which aborts jq with a raw type error on a serde unit variant (a bare JSON string). This file's contract is that undetermined cases abort BY NAME; filtered to objects first so the probe stays total. TWO NEW PRE-FLIGHT CONTROL ARMS, both with negative controls, because no fixture-level arm can witness either property: arm 4 DEFINITION_SHAPES — `_defs` resolves a nested-only description AND a direct-only one, and still ABORTS on one present in neither. arm 5 CARRIER_PRESERVED — an existing canonical carrier survives, while an absent one is still derived. REVERT-PROBE, run: restoring the old `_defs` ⇒ arm 4 reports `nested=FAILED` and t…
🤖 AI text below 🤖
Summary
Replaces the split record-here / refresh-there route for CR 403.3 token battlefield entries with
one authoritative, resume-safe record/emit lifecycle, per @matthewevans' review. The
Suppressfinalize tail now records nothing and parks the entry on
GameState; a single consumer takes thatone owned value and performs
record_zone_change(the sole writer of both CR 400.7 ledgers) and theCR 603.6a event pair together, so a stale row, a lost emit, and CodeRabbit's duplicate
battlefield-entry row are all unrepresentable rather than guarded.
Round 3 closes the reachable trigger defect from @matthewevans' second review. The direct-return
reducer arms —
handle_tribute_choiceand 15 siblings — build theirActionResultinside the matchand never reach
apply_action's tail pipeline, so a copy token realized on those routes previouslyentered with no ETB observer ever seeing it.
apply_action_boundary_corenow converges them throughthe same
run_post_action_pipeline_fromthe tail uses. A second commit makes the CI parse-diffsticky identify the head it was generated from.
Files changed
Trigger convergence (CR 603.6a)
crates/engine/src/game/engine.rs—apply_action_boundary_coreruns the normal post-actionpipeline when the settled gate realized an entry, over the slice the realization appended
crates/engine/src/game/effects/token.rs—realize_settled_token_battlefield_entryreturnswhether it realized; the park,
flush_pending_token_battlefield_entry(sole consumer)crates/engine/src/types/game_state.rs—PendingTokenBattlefieldEntry+ the parkedGameState::pending_token_battlefield_entry;EmitCommittedCopyTokenEntryslimmed to{ object_id }crates/engine/src/game/engine_replacement.rs— the unpaused convergence pointcrates/engine/src/game/effects/counters.rs— the CR 616.1 counter-drain convergence pointcrates/engine/src/game/elimination.rs,crates/engine/src/game/scenario_db.rs,crates/engine/src/game/turns.rs— abandonment and per-turn clearscrates/engine/tests/integration/token_zone_change_index.rs— production regressionscrates/engine/tests/fixtures/cr733/authority_matrix.json.gz— the new field's write-site entryParse-diff head binding
crates/engine/src/bin/coverage_parse_diff.rs—render_markdownemits the head SHA on both thechanged and unchanged branches;
--base-sha/--head-shareject a missing value;parse-diff.jsoncarries both SHAs
Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
Full pipeline, every step in a fresh context. Round 3 (this review cycle): plan → 2 independent
/review-engine-planrounds → implement →/review-impl→ comment-correction pass. Plan/reviewagents ran at
xhigh, implementation agents athigh. Round-3 plan review 1 returned 6 findings(0 blocking); round 2 returned 5 (0 blocking); impl review returned 3 LOW. Every finding was fixed
with code and re-measured. A follow-up commit addresses two CodeRabbit findings on the parse-diff
provenance (below). Earlier rounds are recorded in the previous revision of this body.
Reviewers re-measured rather than trusting reports, and that caught real defects in the work: plan
review corrected three source citations that the previous round's reviewer had signed off as correct
(
engine.rs:7355-7363is theExploreChoicearm, not theCopyTargetChoiceone), and impl reviewrejected the executor's stated proof that the new
boolwas load-bearing as a non-sequitur — theprobes it cited flip pre-existing assertions — then ran a one-bit discriminator that establishes it
properly.
CR references
CR 400.7(the per-turn zone-change ledger) ·CR 403.3(battlefield-entry bookkeeping) ·CR 514.2(the cleanup step ends "this turn" state — the authority for the per-turn reset) ·CR 603.2/CR 603.2c(an ability triggers once per occurrence;turn_zone_change_indexis theengine's own key) ·
CR 603.3b(two same-controller triggers are ordered by their controller — whythe new regression asserts an ordering prompt rather than papering over one) ·
CR 603.4(Fanatic'sintervening-if) ·
CR 603.6a(ETB abilities are checked whenever an event puts a permanent onto thebattlefield — the rule the direct-return arms were violating) ·
CR 614.12a(the as-enters choice ismade before the permanent enters) ·
CR 616.1(competing replacement ordering) ·CR 702.104a/CR 702.104b(Tribute, and "if tribute wasn't paid") ·CR 704.3/CR 704.4(SBA timing) ·CR 704.5f(toughness ≤ 0, the departed-token branch) ·CR 800.4a(a player leaves the game).All grepped in
docs/MagicCompRules.txtbefore use and independently re-verified at the exact linenumbers by the impl reviewer; zero
UNVERIFIED.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— no driftcargo check --workspace --all-targets— exit 0cargo clippy --workspace --all-targets -- -D warnings— exit 0, 0 warningscargo test -p phase-engine --no-fail-fast— 18268 + 12 + 9 + 4374 + 0 = 22663 passed / 0 failed (6+2+7 ignored)cargo test -p phase-ai— 2065 passed / 0 failedParser combinator gate — N/A, no
crates/engine/src/parser/file in the diff (Gate A run anyway, below)The reachable trigger defect, measured on the path @matthewevans named
(
CopyTargetChoice → NamedChoice → TributeChoice, Soul Warden on the battlefield):bee984f80The
+1is produced byengine_priority::run_post_action_pipeline_fromat the action boundary — thesame function
run_post_action_pipelineitself delegates to — over the slice the realizationappended, not by an alternate route. Verified by reverting the new block: the regression fails at its
reach-guard first (
left: […"TributeChoice"] right: […"TributeChoice","OrderTriggers(2)"]), and withthe guard relaxed so execution proceeds, the life assertion alone fails
left: 0, right: 1— exactlythe value the review called out. The fix is arm-agnostic: the reducer match has 16 non-error direct
returns and the convergence is inert on the 15 that never park.
The regression also pins
"OrderTriggers(2)". The(2)is the discriminating part — Tribute isdeclined, so Fanatic's own CR 702.104b intervening-if is true and its ETB fires alongside Soul
Warden's; both are P0's, so CR 603.3b requires an ordering prompt. The sibling routes (Painter,
Watchdog) keep full-vector prompt assertions with no
OrderTriggerselement, so a spurious orderingprompt on those routes fails them.
Revert probes, each actually run and restored byte-identically (md5-verified), including four run
independently by the reviewer:
0 → 1are retained for CR 704.3 SBA ordering and for a counter drain that does not settle in its own action)
false⇒ fails at exactly the arm-iii assertion, nothing elsescan_from → 0⇒ nothing flips (see the disclosure below)left: (0, 0) right: (1, 1)Emitpositive control stays greenwriteln!⇒ bothcontains(head)assertions flip; move it above themarker ⇒ the marker-first assertion flips
Save/load compatibility for the
GameStatefield — measured, not assumed.GameStatecarries nodeny_unknown_fields, and the field is#[serde(default, skip_serializing_if = "Option::is_none")],so an absent key loads as
Noneand aNonevalue is never written back. Census: the repo ships11 real
{"gameState": …}dumps as.json.gzfixtures, all predating this field;pending_token_battlefield_entryoccurs 0 times in every one of them. Every test thatdeserializes one of those dumps passes: 132 passed / 0 failed, plus a serde round-trip unit test.
No compatibility shim, no pristine-dump migration.
Rebased onto
upstream/main36ac6d2a1before pushing. That range touchesengine.rs, whichthis PR modifies, so the full gate set above was re-run after the rebase rather than reused: the
rebase itself was conflict-free, and the post-rebase numbers are the ones reported here (the counts
rose from the pre-rebase run because upstream added tests).
Gate A
Gate A PASS head=bc9f9445524f90c16cbbe27ae720d644fe4da723 base=36ac6d2a1ccb4c891a20403557dfe5ac296a7022
Anchored on
run_post_action_pipeline_fromat anested-slice seam; it has five existing production call sites for exactly this problem
(
engine_payment_choices.rs:1249/1317/1339,stack.rs:2647,engine_resolution_choices.rs:543),and the new boundary call is the same shape. The
state.waiting_for = wf.clone(); result.waiting_for = wf;pair mirrors
apply_action's own post-pipeline sync verbatim — both writes are required, for twodifferent reasons:
finish_action_boundary→sync_waiting_forcopiesresultintostate, so astate-only write is undone, and the life-safety preview never calls
finish_action_boundary, so aresult-only write is insufficient
pending_liminal_entry_resume, the existingOption<…>mid-resolution park onGameStatewith#[serde(default, skip_serializing_if = "Option::is_none")];pending_token_battlefield_entrymirrors it field attribute for field attributeFinal review-impl
Final review-impl PASS head=bc9f9445524f90c16cbbe27ae720d644fe4da723
Claimed parse impact
None.
Scope Expansion
One, requested by the reviewer as required evidence. The
<!-- coverage-parse-diff -->stickycould not be made to identify the head by regenerating it, because no generator branch had ever
emitted a head SHA.
render_markdown(crates/engine/src/bin/coverage_parse_diff.rs) returned earlyfor the no-change case with the marker and
✓ No card-parse changes detected.and nothing else — nobase, no head — while the changed-cards branch printed only the baseline. Comment
5153742352didre-run for
bee984f80(updated_at 2026-08-02T06:57:35Z); its body is byte-identical to that earlyreturn. So the sticky is now head-bound on both branches. This is a class-wide provenance fix, not
a #6851 one.
That generator is ordinary crate source, so no workflow file is touched. The head is sourced from the
HEAD_SHAthe CI step already exports, with a--head-shaflag mirroring the existing--base-sha.A
git rev-parse HEADfallback would have been wrong rather than merely inelegant — that job checksout the synthetic PR merge commit, so git reports the merge SHA, never
pull_request.head.sha. Twoconstraints shaped the wording, both pinned by assertions with revert-probes:
scripts/pr_review.pyrequires the marker to stay the first line, and it classifies sticky state by the substrings
"Baseline pending"and"signature(s)", so the head line must contain neither or a no-change stickymisclassifies as real parse changes.
CodeRabbit then raised two findings on that generator, both taken. A present-but-valueless
--base-sha/--head-shasilently fell back, so a report could name a commit it does not describe;both now error. The pre-existing
--base-shaline was fixed alongside the one this branch added —leaving it lenient would put two flags in the same provenance category on different rules. The
output flags stay lenient and a test pins that asymmetry as deliberate.
parse-diff.jsoncarried noprovenance at all while both the report and the sticky send a truncated reader to open it, so it now
carries both SHAs; nothing in the repo deserializes that artifact (its four references are two
workflow write/upload paths and two prose mentions), so added keys break no consumer. Neither defect
is reachable through CI, which never passes
--head-shaand always pairs its flags with values —this hardens the local and manual surface, and is not a CI bug fix.
Two pre-existing defects found while measuring are filed separately rather than folded in: (1) a stale
CopyTargetChoiceafter a successful copy entry routes the entry event tocollect_triggers_into_deferred, swallowing ETB observers on copy-token entries (reproducesidentically with and without this diff); (2) conceding at the as-enters pause panics on unmodified
mainatelimination.rs:1069.Validation Failures
The round-2 disclosure that the Tribute two-pause class realizes after its trigger scan is retired
— that defect is fixed, not disclosed. Remaining disclosures, none blocking:
Err-restore arm of the new boundary block is unmeasured. It mirrors the existingrestore-on-
Errarm a few lines above it, but no test drives a failing post-action pipeline there.scan_fromvs0has a measured mechanism but no discriminating test. Widening to0flipsnothing today. It is still wrong:
collect_and_drain_observer_triggers_if_settlednever writesconsumed_before_priority_trigger_events, and the exclusion filter coversZoneChangedonly, soevery non-
ZoneChangedobserver event it already collected would be collected twice.event_start = scan_from, the direct-returnhandler's own events — from
effects::tribute::apply_paid, and fromresume_pending_continuation_if_priority, which can resolve a whole ability chain — still get noCR 603.2 scan. That was true before this change too; it is not a regression, but it means "converges
through the normal pipeline" is exact about the realized entry and not about everything the handler
emitted.
TributeChoiceshape.abandon_source_bound_resolution_promptclears the park, but its gate matches onlyWaitingFor::NamedChoiceandWaitingFor::OpponentGuess, so the Painter sibling is safe byconstruction and Tribute is not. Unmeasured; adjacent to the separately-filed concede panic above.
apply_interaction_pre_reconciliation_for_life_safetydocuments itself as "exactly the reducer portion" and returns without
finish_action_boundary; on aparked-entry route it would now also run the pipeline. Not reachable today — that root cannot
produce a parked copy-token entry, and it operates on a clone.
.github/workflows/**, which this PR treats as a hard stop. (a)ci.yml's baseline-not-publishedfallback body names the base only; adding the head there needs one printf argument. (b) When the
engine-source hash is unchanged the step sets
produced=false, so no artifact is uploaded and thesticky silently retains its previous head's body — no generator change can bind a comment that was
never regenerated. Offered as text in the review thread for a maintainer to apply.
A note on the required-evidence item. The sticky names the head it was generated from, so landing
these commits necessarily moves it: the regenerated comment will identify the new head, not
bee984f80. A literal identity check againstbee984f80…is unsatisfiable by construction; thesubstantive requirement — that the artifact be bound to a head — is met.
CI Failures
None.
Pre-existing and untouched:
CR733_CENSUS_STRICT=1fails oncensus.site_count(pinned 3597; acensus run on the pristine pre-rework tree
057d80a65gives 3669), so that pin was already stalebefore this rework. The always-on
cr733_authority_matrix_covers_the_fresh_write_censusgate isgreen. No
GameStatefield is added in this round, so the matrix is unchanged.Summary by CodeRabbit
Bug Fixes
Tools